Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement selective optimization levels on a per-dependency basis. #1826

Closed
wants to merge 2 commits into from

Conversation

pcwalton
Copy link

Inside the dependencies.foo section, you may now specify a minimum
optimization level to be applied to this dependency. If multiple such
optimization levels are supplied, Cargo chooses the highest one.

This will probably need to go through an RFC before landing.

Addresses #1359.

r? @alexcrichton -- would be interested to hear design thoughts

Inside the `dependencies.foo` section, you may now specify a minimum
optimization level to be applied to this dependency. If multiple such
optimization levels are supplied, Cargo chooses the highest one.

This will probably need to go through an RFC before landing.

Addresses rust-lang#1359.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

pcwalton added a commit to pcwalton/servo that referenced this pull request Jul 24, 2015
… that of

Gecko in debug mode.

One WPT test is still too slow. No reasonable timeout value made it fast
enough. Therefore, I have marked the test as a timeout. Optimizing during WPT
running or landing rust-lang/cargo#1826 would fix this as well, I suspect.
@alexcrichton
Copy link
Member

Thanks for the PR @pcwalton! I talked about something like this in the past with @wycats and the conclusion we've reached is that the specification here is probably best done at the top level, not in the dependencies themselves. In general it seems best if you have complete control over how your dependencies are built rather than having a transitive dependency deciding how it's compiled. One example of this is that the logic for "pick the highest level" isn't always correct as there may be some dependencies you want to force to a lower optimization level.

This issue about configuring dependencies from the top-level definitely comes up quite a bit (#1359, #629, #942), and it's probably best to enable solving all of these in basically one fell swoop. Along those lines I think that we'll want to implement something along the lines of:

[dependencies-config]
foo = { profile = "release" }
"bar:0.1.4" = { override = "path/to/override" }
"crates.io/foo" = { crate-type = [ "..." ] }

This allows you to:

  • At the top level have control over how each dependency is compiled
  • Reference existing profiles to leverage all the other options in profiles
  • Specify any arbitrary package in the dependency graph unambiguously

Some of these details I think still need to be bike-shedded and this may warrant an RFC as well as it's a somewhat substantial change.

@pcwalton
Copy link
Author

OK, I'll work on this ASAP. I am blocked on this so there is no other option.

@bors
Copy link
Contributor

bors commented Jul 27, 2015

☔ The latest upstream changes (presumably #1848) made this pull request unmergeable. Please resolve the merge conflicts.

@alexcrichton
Copy link
Member

This has been inactive for awhile (and I think Servo is working for now), so I'm going to close this in favor of issue like #942 which should provide a nice vector for us to implement this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants